Improve types on AssetIssuance struct#289
Open
apoelstra wants to merge 4 commits into
Open
Conversation
Somehow I neglected to do this in ElementsProject#278
…nonce Previously we had been using the secp265k1_zkp::Tweak type to represent a blinding nonce in an issuance. This had a number of issues: * `Tweak` is a low-level crypto type which is ambiguous about whether or not it's secret or public, and doesn't have any meaning beyond "value you can add to a secp point" * In particular the special `ZERO_TWEAK` constant was interpreted to mean "new issuance" and this was implicit throughout the codebase. * As a foreign type, we could not directly implement any encoding or decoding traits on this; we had to convert it to/from a byte array. * As a "general" type, it doesn't distinguish between various uses of tweak, which can lead to dangerous type confusion. * `Tweak` cannot represent values that are out of range of the secp256k1 group, while technically Elements permits such transactions to exist (e.g. `decoderawtransaction` will accept them), even though it will fail VerifyAmounts. (I carefully checked all the codepaths: if the amounts in an issuance are null, so much be the nonce (or else you get a "superfluous issuance" error; if they're non-null and the blinding nonce is zero (which is in-range), it's a new issuance; if it's not zero then it's put into secp256k1_generator_generate_blinded which checks for overflow as its very first check.)
The only place this is actually "consensus encoded" is arguably in the pegin witness stack, but that's reason enough to have these.
Member
Author
|
I think after this we should cut a 0.27 release of rust-elements. I have a bunch of breakage related to encoding coming down the pike and it'd be good to have a release right before and after that to try to contain the damage for people updating their stuff. |
philipr-za
reviewed
Jul 23, 2026
Contributor
There was a problem hiding this comment.
Changes look good and principled. Tests pass locally, ACK 02122be.
Some minor editorial comments.
| @@ -1,4 +1,4 @@ | |||
| // Rust Elements Library | |||
| // Rust Elements Libraryss | |||
Contributor
There was a problem hiding this comment.
typo
Suggested change
| // Rust Elements Libraryss | |
| // Rust Elements Librarys |
| } | ||
|
|
||
| encoding::encoder_newtype_exact! { | ||
| /// Encoder for the [`OutPoint`] type. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// Encoder for the [`OutPoint`] type. | |
| /// Encoder for the [`AssetEntropy`] type. |
| } | ||
|
|
||
| encoding::encoder_newtype_exact! { | ||
| /// Encoder for the [`OutPoint`] type. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// Encoder for the [`OutPoint`] type. | |
| /// Encoder for the [`AssetBlindingNonce`] type. |
| } | ||
|
|
||
| encoding::encoder_newtype_exact! { | ||
| /// Encoder for the [`OutPoint`] type. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// Encoder for the [`OutPoint`] type. | |
| /// Encoder for the [`AssetId`] type. |
|
|
||
| use crate::{encode::serialize, pset::PartiallySignedTransaction}; | ||
| use crate::issuance::AssetBlindingNonce; | ||
| use crate::{encode::serialize, pset::PartiallySignedTransaction}; |
Contributor
There was a problem hiding this comment.
Suggested change
| use crate::{encode::serialize, pset::PartiallySignedTransaction}; | |
| use crate::{encode::serialize, pset::PartiallySignedTransaction}; |
indenting
| /// used to blind the reissuance token (which must be blinded in order to be | ||
| /// spent, due to a quirk in the Elements consensus code.) | ||
| /// | ||
| /// Conceputally this can be thought of as an `Option<AssetBlindingFactor>`, except |
Contributor
There was a problem hiding this comment.
Suggested change
| /// Conceputally this can be thought of as an `Option<AssetBlindingFactor>`, except | |
| /// Conceptually this can be thought of as an `Option<AssetBlindingFactor>`, except |
delta1
approved these changes
Jul 23, 2026
| use std::str::FromStr; | ||
|
|
||
| use crate::{encode::serialize, pset::PartiallySignedTransaction}; | ||
| use crate::issuance::AssetBlindingNonce; |
Member
There was a problem hiding this comment.
nit: unnecessary since super::* is imported below
| /// This is something of a dangerous function, since in general blinding factors should | ||
| /// be considered secret data, while blinding nonces are public (they are encoded on | ||
| /// the blockchain). So callers of this function should be sure that this is a blinding | ||
| /// factor that they intend to reveal.) |
Member
There was a problem hiding this comment.
Suggested change
| /// factor that they intend to reveal.) | |
| /// factor that they intend to reveal. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use strong types for asset entropy and blinding nonce.